Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow modification of existing attributes and text content. #23

Merged
merged 5 commits into from
Jan 15, 2024
Merged

Allow modification of existing attributes and text content. #23

merged 5 commits into from
Jan 15, 2024

Conversation

cohenerickson
Copy link
Contributor

Ignoring my previous issue, I ended up stumbling across an issue when modifying the value of attributes that were already attached to an element, as well as modifying existing text content.

The previous behavior for setAttribute was to simply append a new attribute to the attrs array, while this does work for new elements, it causes issues when attempting to override existing attributes. When parse5 serializes the AST, it only recognizes the first attribute of a type, and ignores any others with the same name. This behavior is un-intuitive as calling setAttribute in some situations may have no effect on the serialized content.

This issue is easily fixed by simply removing any old attributes with the given name.

I also stumbled across a very similar issue with setTextContent when modifying the text content of some elements. This issue was also caused by appending new text nodes without removing any previous content beforehand. While the old behavior does act similar to the document.write() function, the name setTextContent doesn't match the functionality. If this behavior is intended, I would recommend renaming setTextContent to appendTextContent and creating a new function for setTextContent which behaves intuitively.

This issue was also easily fixed by removing any text nodes from a parent element before appending the new content.

src/text.ts Outdated Show resolved Hide resolved
src/text.ts Outdated Show resolved Hide resolved
Copy link
Member

@43081j 43081j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, thanks a lot for taking the time!

will get a new version published some time this week

@43081j 43081j merged commit b7481ff into parse5:main Jan 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants